Add INA219 usermod support and update platformio.ini#361
Open
Topchris18 wants to merge 6 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR adds INA219 I2C current/power sensor support via a new WLED usermod, registers the usermod ID, and adds an optional Adafruit INA219 library entry in platformio.ini. ChangesINA219 Usermod Implementation
Build Integration & ID
Project Configuration
Sequence Diagram(s)sequenceDiagram
participant WLED as WLED System
participant Usermod as UsermodINA219
participant I2C as I2C Bus
participant INA219 as INA219 Sensor
participant JSON as Config/JSON
WLED->>Usermod: setup()
Usermod->>I2C: joinWire()
Usermod->>INA219: instantiate(address) & begin()
Usermod->>INA219: applyCalibration(busVoltageRange, maxCurrentRange)
INA219-->>Usermod: ready / not found
loop every readInterval
WLED->>Usermod: loop()
Usermod->>INA219: readShuntVoltage()
INA219-->>Usermod: V_shunt
Usermod->>INA219: readBusVoltage()
INA219-->>Usermod: V_bus
Usermod->>Usermod: calculate current & power (using shuntResistor_mOhm)
Usermod-->>JSON: addToJsonInfo(measurements)
end
WLED->>Usermod: readFromConfig()
JSON-->>Usermod: settings
Usermod->>Usermod: reinit or reapply calibration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@platformio.ini`:
- Around line 297-298: The new global dependency line "adafruit/Adafruit INA219
@ 1.2.1" was added under the global [env] and therefore affects all build
environments; revert this change from the global section and either move the
dependency into the specific environment(s) that need the INA219 or leave it
commented with a clear TODO requiring explicit maintainer/WLED org approval
before enabling. Ensure you reference the dependency string "adafruit/Adafruit
INA219 @ 1.2.1" when making the change and add a brief sign-off note (e.g.,
"Requires maintainer approval") so reviewers can approve before it becomes
active.
In `@usermods/INA219_v2/usermod_ina219.h`:
- Around line 119-120: The computation of current (current_mA = shuntVoltage_mV
/ (shuntResistor_mOhm / 1000.0f)) can divide by zero or a tiny value if
shuntResistor_mOhm is misconfigured; validate and clamp shuntResistor_mOhm when
loading config (where it’s set) to a safe minimum (e.g. >0 and not near-zero)
and add a runtime guard before computing current in the measurement path (check
shuntResistor_mOhm against the minimum, skip or set current_mA/power_mW to 0 and
log/warn if invalid). Ensure you update symbols: shuntResistor_mOhm validation
at config load and the check around current_mA/power_mW calculation that uses
shuntVoltage_mV and loadVoltage_V.
- Around line 62-69: applyCalibration currently favors 32V calibrations whenever
maxCurrentRange_A > 0.4, ignoring a configured busVoltageRange_V of 16V; update
applyCalibration to first branch on busVoltageRange_V (e.g., if
busVoltageRange_V <= 16) and then choose the correct current-based calibration
(call ina219->setCalibration_16V_400mA for <=0.4A else a 16V+higher-current
routine if available), otherwise for busVoltageRange_V > 16 choose the 32V
calibrations (use ina219->setCalibration_32V_1A or ina219->setCalibration_32V_2A
based on maxCurrentRange_A); make the same voltage-first decision wherever the
settings from busVoltageRange_V/maxCurrentRange_A are applied (references:
function applyCalibration, variables busVoltageRange_V and maxCurrentRange_A,
and methods ina219->setCalibration_16V_400mA, ina219->setCalibration_32V_1A,
ina219->setCalibration_32V_2A).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9ca8f889-7b42-4cb8-858e-7c6fa6fed515
📒 Files selected for processing (4)
platformio.iniusermods/INA219_v2/usermod_ina219.hwled00/const.hwled00/usermods_list.cpp
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
usermods/INA219_v2/usermod_ina219.h (2)
43-43:lastMeasureduplicates base-classlastTime.
Usermodalready providesunsigned long lastTime. Reusing it would slightly shrink the footprint and match convention across usermods. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/INA219_v2/usermod_ina219.h` at line 43, The field lastMeasure duplicates the base-class Usermod's lastTime; remove the unsigned long lastMeasure declaration from usermod_ina219.h and update all usages in the INA219 usermod (references to lastMeasure in methods like setup(), loop(), read/measure routines) to use lastTime instead so the mod reuses the inherited timer field and reduces footprint while preserving behavior.
217-222: PROGMEM static member definitions in header — potential ODR risk if header is included elsewhere.Defining
UsermodINA219::_readInterval[]et al. in the header is currently safe because this header is included only once (viausermods_list.cppwith#ifdef USERMOD_INA219guard). However, if the header is ever included from a second translation unit (e.g., tests, a future companion.cpp, or another usermod), you'll get multiple-definition linker errors.Consider marking them
inline(C++17) to allow safe multi-inclusion, or move them to a companion.cpp. Other usermods in the repo use this pattern, so this is optional.♻️ Proposed change (C++17)
-const char UsermodINA219::_readInterval[] PROGMEM = "readInterval-ms"; -const char UsermodINA219::_i2cAddress[] PROGMEM = "i2cAddress"; -const char UsermodINA219::_shuntResistor[] PROGMEM = "shuntResistor-mOhm"; -const char UsermodINA219::_maxCurrentRange[] PROGMEM = "maxCurrentRange-A"; -const char UsermodINA219::_busVoltageRange[] PROGMEM = "busVoltageRange-V"; +inline const char UsermodINA219::_readInterval[] PROGMEM = "readInterval-ms"; +inline const char UsermodINA219::_i2cAddress[] PROGMEM = "i2cAddress"; +inline const char UsermodINA219::_shuntResistor[] PROGMEM = "shuntResistor-mOhm"; +inline const char UsermodINA219::_maxCurrentRange[] PROGMEM = "maxCurrentRange-A"; +inline const char UsermodINA219::_busVoltageRange[] PROGMEM = "busVoltageRange-V";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/INA219_v2/usermod_ina219.h` around lines 217 - 222, The PROGMEM static definitions (UsermodINA219::_readInterval, _i2cAddress, _shuntResistor, _maxCurrentRange, _busVoltageRange) in the header can cause ODR multiple-definition errors if the header is included in more than one TU; fix by either marking these static data definitions inline (C++17) or moving them into a companion .cpp file where they are defined once; update the header to declare them (if moving) or add the inline specifier to each definition and keep the PROGMEM storage so UsermodINA219 still references the same symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@usermods/INA219_v2/usermod_ina219.h`:
- Around line 201-210: readFromConfig() currently only calls setup() when the
I2C address changes, which misses the case where enabled transitions from
false→true; update readFromConfig() to track the previous enabled state (e.g.
oldEnabled) and if oldEnabled != enabled and enabled == true call setup() to
force sensor initialization (use same setup() that handles I2C init and sets
sensorFound/ina219), and when enabled transitions to false clear or reset
sensorFound and ina219 to avoid stale state; keep the existing address-change
logic but ensure both paths handle the enabled flag consistently.
- Around line 190-199: After reading JSON into i2cAddress, readInterval and
busVoltageRange_V via getJsonValue, add validation and clamping: validate
i2cAddress against the supported INA219 addresses (0x40,0x41,0x44,0x45) and if
not one of those set a safe default (e.g. 0x40) and log the correction; ensure
readInterval is >= 1 (or a chosen minimum) and if zero or too small set it to
the minimum and log; for busVoltageRange_V only accept 16 or 32 (clamp or
default to 32) and log adjustments. Place these checks immediately after the
getJsonValue lines (near variables i2cAddress, readInterval, busVoltageRange_V)
so begin() will receive valid values. Ensure configComplete remains accurate if
values were corrected and keep the existing shuntResistor_mOhm min clamp logic.
---
Nitpick comments:
In `@usermods/INA219_v2/usermod_ina219.h`:
- Line 43: The field lastMeasure duplicates the base-class Usermod's lastTime;
remove the unsigned long lastMeasure declaration from usermod_ina219.h and
update all usages in the INA219 usermod (references to lastMeasure in methods
like setup(), loop(), read/measure routines) to use lastTime instead so the mod
reuses the inherited timer field and reduces footprint while preserving
behavior.
- Around line 217-222: The PROGMEM static definitions
(UsermodINA219::_readInterval, _i2cAddress, _shuntResistor, _maxCurrentRange,
_busVoltageRange) in the header can cause ODR multiple-definition errors if the
header is included in more than one TU; fix by either marking these static data
definitions inline (C++17) or moving them into a companion .cpp file where they
are defined once; update the header to declare them (if moving) or add the
inline specifier to each definition and keep the PROGMEM storage so
UsermodINA219 still references the same symbols.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f446a9fc-73ad-45d1-bc7d-bee6245ef75e
📒 Files selected for processing (2)
platformio.iniusermods/INA219_v2/usermod_ina219.h
✅ Files skipped from review due to trivial changes (1)
- platformio.ini
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
usermods/INA219_v2/usermod_ina219.h (1)
198-201:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMark config as incomplete when clamping invalid shunt values.
When the shunt value is corrected,
configCompleteshould be set tofalseso the sanitized value can be persisted.🛠️ Suggested patch
if (shuntResistor_mOhm < 1.0f) { USER_PRINTLN(F("[INA219]: shuntResistor-mOhm clamped to minimum 1 mOhm.")); shuntResistor_mOhm = 1.0f; + configComplete = false; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@usermods/INA219_v2/usermod_ina219.h` around lines 198 - 201, The code clamps shuntResistor_mOhm to 1.0f but doesn't mark the configuration as needing persistence; inside the branch that adjusts shuntResistor_mOhm (the if (shuntResistor_mOhm < 1.0f) block) set configComplete = false so the sanitized value will be saved; update the same block where USER_PRINTLN is called (referencing shuntResistor_mOhm and configComplete) to ensure the corrected value is persisted.
🧹 Nitpick comments (1)
usermods/INA219_v2/usermod_ina219.h (1)
80-83: ⚡ Quick winRelease INA219 state when the usermod is disabled.
This early return leaves
ina219allocated andsensorFoundstale while disabled. Clearing them here keeps state consistent and frees RAM.♻️ Suggested patch
void setup() override { if (!enabled) { + if (ina219) { + delete ina219; + ina219 = nullptr; + } + sensorFound = false; initDone = true; // mark as initialized so re-enabling via UI triggers (re)setup return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@usermods/INA219_v2/usermod_ina219.h` around lines 80 - 83, When handling the early-return branch in the enabled check, release the INA219 state: if enabled is false, delete or free the ina219 object (or call its destructor/cleanup) and set sensorFound to false (and any related flags) before setting initDone = true and returning; update the block around the enabled check that currently contains initDone = true; return; to explicitly clear ina219 and reset sensor-related state so RAM is freed and state remains consistent when the usermod is re-enabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@usermods/INA219_v2/usermod_ina219.h`:
- Around line 198-201: The code clamps shuntResistor_mOhm to 1.0f but doesn't
mark the configuration as needing persistence; inside the branch that adjusts
shuntResistor_mOhm (the if (shuntResistor_mOhm < 1.0f) block) set configComplete
= false so the sanitized value will be saved; update the same block where
USER_PRINTLN is called (referencing shuntResistor_mOhm and configComplete) to
ensure the corrected value is persisted.
---
Nitpick comments:
In `@usermods/INA219_v2/usermod_ina219.h`:
- Around line 80-83: When handling the early-return branch in the enabled check,
release the INA219 state: if enabled is false, delete or free the ina219 object
(or call its destructor/cleanup) and set sensorFound to false (and any related
flags) before setting initDone = true and returning; update the block around the
enabled check that currently contains initDone = true; return; to explicitly
clear ina219 and reset sensor-related state so RAM is freed and state remains
consistent when the usermod is re-enabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 87b858a7-5069-4c9a-9e87-458fc3a679f9
📒 Files selected for processing (1)
usermods/INA219_v2/usermod_ina219.h
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wled00/const.h`:
- Line 313: The PR includes unrelated changes that must be separated or
justified: either remove/revert modifications to WLED_NUM_ETH_TYPES (symbol
WLED_NUM_ETH_TYPES and updated board defines WLED_ETH_OLIMEX_GTW,
WLED_ETH_QUINLED_V4_UNOQUAD, WLED_ETH_QUINLED_V4_OCTA), the error-code
renames/values (ERR_PERSISTENT, ERR_REBOOT_NEEDED, ERR_POWEROFF_NEEDED), and the
new LED bounds checks/static_asserts (MAX_LEDS validation and references to
MAX_LEDS_PER_BUS), or split them into dedicated PRs with clear descriptions; if
any of these edits are required for INA219 usermod, add a concise rationale in
this PR describing why each specific change (referencing the exact symbols above
and the static_assert additions) is necessary for INA219 support.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ac08c364-3236-4cfa-b871-4cc123b6cde3
📒 Files selected for processing (1)
wled00/const.h
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
Collaborator
@coderabbitai this seems to be a false positive, because the changes you mention were added by my recent "update from |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Look at const.h around line 313
sed -n '300,330p' wled00/const.h
echo "---"
# Check PR commits to see which came from mdev update
gh pr view 361 --repo MoonModules/WLED-MM --json commits --jq '.commits[] | {oid: .oid, messageHeadline: .messageHeadline, authorName: .authors[0].name}'Length of output: 2101
🧠 Learnings used |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Collaborator
@Topchris18 please check if this recommendation makes sense. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores